Skip to content

Comments

fix: Fix yq syntax for parsing the metadata XML content#27

Closed
cheemeng wants to merge 2 commits intomise-plugins:mainfrom
cheemeng:main
Closed

fix: Fix yq syntax for parsing the metadata XML content#27
cheemeng wants to merge 2 commits intomise-plugins:mainfrom
cheemeng:main

Conversation

@cheemeng
Copy link

There must have been some changes lately in the XML content... which led to parsing error when trying to get version numbers of the SDK.

@Syquel
Copy link
Contributor

Syquel commented Oct 19, 2025

Thanks a lot for finding and investigating this issue!

I dug around a little bit: The issue is not with the XML file but with yq which just recently received fixed support for XML namespaces in v4.48.1.
Although your PR is totally correct and would be the right way to implement it it would unfortunately break this plugin for all users who use yq 4.47 or lower, because of the bugged handling of XML namespaces there.

To restore the "bugged" behavior in yq v4.48.1 we need to add the flag --xml-keep-namespace=false instead of adding the XML namespace to our yq command.
IF you agree with this approach it would be great if you could amend your PR to use that solution.

@cheemeng
Copy link
Author

Thanks a lot for finding and investigating this issue!

I dug around a little bit: The issue is not with the XML file but with yq which just recently received fixed support for XML namespaces in v4.48.1. Although your PR is totally correct and would be the right way to implement it it would unfortunately break this plugin for all users who use yq 4.47 or lower, because of the bugged handling of XML namespaces there.

To restore the "bugged" behavior in yq v4.48.1 we need to add the flag --xml-keep-namespace=false instead of adding the XML namespace to our yq command. IF you agree with this approach it would be great if you could amend your PR to use that solution.

Ah... sorry, guess my sleuthing did not go as deep as yours to check if it was something with yq instead :-)

In that case it would be much better to cancel this PR and submit a new one with the flag instead, so as not to pollute the commits with undos?

@Syquel
Copy link
Contributor

Syquel commented Oct 19, 2025

Your proposed solution with the added XML namespace just gave me the idea to look into yq, so thanks for that :)

You can just amend your existing commit and force-push to your branch again.
The PR is updated automatically then with the amended commit and force-pushing is fine IMO as long as its on a feature / bugfix branch.

I just took a look again and the tests will need to be adjusted to disregard namespaces too.
Did not check any further but at least the following lines need to be adjusted:

No guarantee of accuracy or completeness ;)

If you want I can create the PR with the changes outlined above, but after the work you put in it would be great to have your name in the git history of this plugin.

@cheemeng
Copy link
Author

Oops, just saw your new message and got reminded about the test scripts as well... gimme a sec

@cheemeng cheemeng changed the title Fix yq syntax for parsing the metadata XML content fix: Fix yq syntax for parsing the metadata XML content Oct 19, 2025
@cheemeng
Copy link
Author

Erm... on second thought, I am finding it hard to read the test scripts and amend the tests you suggested (am traveling at the moment, and the github mobile app is driving me nuts now lol), perhaps you could help with that @Syquel ?

@Syquel
Copy link
Contributor

Syquel commented Oct 19, 2025

I submitted #28

@cheemeng
Copy link
Author

Lol I finally managed to find a place with wifi to sit down and amend the tests on my laptop... again, only to find that you have done it already. Thanks @Syquel

Copy link
Author

@cheemeng cheemeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it might make more sense to keep the --xml-keep-namespace parameter next to --input-format xml because the option only makes sense when it is an XML anyway

@Syquel
Copy link
Contributor

Syquel commented Oct 19, 2025

Ah sorry I thought you wanted me to take over :/

I am still okay with your PR if you rebase it instead of merging the main branch into yours, but that's jdx' decision to make in the end.

@cheemeng
Copy link
Author

It's all good no worries. I felt bad to push it back to you to do, that was why I did it as well. At the end of the day it matters not whose code is used, as long as they fix the issue, and fix it correctly. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants